Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add host parameter to telemetry config #1033

Merged
merged 3 commits into from
Jun 3, 2021
Merged

Add host parameter to telemetry config #1033

merged 3 commits into from
Jun 3, 2021

Conversation

andynog
Copy link
Contributor

@andynog andynog commented Jun 1, 2021 •
edited by romac
Loading

Closes: #1032

Description

Add host parameter to telemetry config section, to allow access from a different machine in the network (eg. by using 0.0.0.0 as the host).

For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@andynog andynog added A: bug Admin: something isn't working I: telemetry Internal: related to Telemetry & metrics labels Jun 1, 2021
@andynog andynog added this to the 05.2021 milestone Jun 1, 2021
@andynog andynog requested review from romac and ancazamfir June 1, 2021 20:10
@andynog andynog requested a review from adizere as a code owner June 1, 2021 20:10
mircea-c
mircea-c reviewed Jun 1, 2021
View reviewed changes
telemetry/src/server.rs Outdated Show resolved Hide resolved
@romac romac changed the title Fixing telemetry server listen host (#1032) Add host parameter to telemetry config Jun 2, 2021
@romac romac mentioned this pull request Jun 2, 2021
Merged
9 tasks
@romac romac added needs guide update and removed A: bug Admin: something isn't working labels Jun 2, 2021
@ancazamfir ancazamfir removed this from the 05.2021 milestone Jun 2, 2021
@romac romac requested a review from mircea-c June 2, 2021 20:43
andynog
andynog commented Jun 2, 2021
View reviewed changes
Copy link
Contributor Author

@andynog andynog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, the only suggestion is that if a more meaningful message could be added when exiting. The message "Address in use" does not have a context and for an operator is not helpful because it doesn't say the telemetry server failed to start because it's trying to use a port which is already allocated. If there's a way to at least say telemetry server or something like that I think that would be helpful than just bubbling up the error from the web server

Error: Hermes failed to start, last error: Address already in use (os error 48)

@romac
Copy link
Member

romac commented Jun 2, 2021

It says right before exiting with this message:

Jun 02 22:32:48.130 ERROR ibc_relayer_cli::commands::start: telemetry service failed to start: Address already in use (os error 48)

which is not perfect but at least provides some more context. Is that enough or would you want it to also mention this in the final error? Maybe we could introduce an enum to capture the server error and then print it accordingly?

@andynog
Copy link
Contributor Author

andynog commented Jun 2, 2021

It says right before exiting with this message:

Jun 02 22:32:48.130 ERROR ibc_relayer_cli::commands::start: telemetry service failed to start: Address already in use (os error 48)

which is not perfect but at least provides some more context. Is that enough or would you want it to also mention this in the final error? Maybe we could introduce an enum to capture the server error and then print it accordingly?

It says right before exiting with this message:

Jun 02 22:32:48.130 ERROR ibc_relayer_cli::commands::start: telemetry service failed to start: Address already in use (os error 48)

which is not perfect but at least provides some more context. Is that enough or would you want it to also mention this in the final error? Maybe we could introduce an enum to capture the server error and then print it accordingly?

Sorry, my bad. Indeed it says. I thought that ERROR line was only shown depending on the log_level parameter in the config but that's not the case, it will show in any case. So that's all good.

Thanks!

@romac romac merged commit 7171f0e into master Jun 3, 2021
@romac romac deleted the andy/telemetry-srv-fix branch June 3, 2021 11:41
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@andynog @romac
Add host parameter to telemetry config (informalsystems#1033)
08f9999 
* Fixing telemetry server listen host (informalsystems#1032)

* Add `host` parameter for the telemetry server

* Gracefully exit when telemetry service fails to start

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adizere adizere adizere approved these changes

@romac romac Awaiting requested review from romac

@ancazamfir ancazamfir Awaiting requested review from ancazamfir

@mircea-c mircea-c Awaiting requested review from mircea-c

Assignees
No one assigned
Labels
I: telemetry Internal: related to Telemetry & metrics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot reach telemetry endpoint from another machine in the same network
5 participants
@andynog @romac @adizere @ancazamfir @mircea-c